fix(sandbox): give each Hermes sandbox its own OpenAI-compatible API port - #8577

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

agents/hermes/mcp-config-transaction.py (1)

102-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The Hermes API port range is duplicated as bare literals across the Python runtime. No Python module names the range or the default. Each site repeats 8642 and 8652 inline, so a future range change must be found and applied in every copy. A third copy exists in agents/hermes/plugin/__init__.py _hermes_api_port, which repeats the same two literals four times. The stack outline places the authoritative contract in src/lib/core/ports.ts, and no Python counterpart mirrors it.

Introduce named module constants, for example HERMES_API_PORT_MIN, HERMES_API_PORT_MAX, and HERMES_DEFAULT_API_PORT, and reference them at each site.

• agents/hermes/mcp-config-transaction.py#L102-L112: replace the inline 8642 <= port <= 8652 bound in _parse_gateway_public_port with the named constants, and define those constants next to GATEWAY_INTERNAL_PORT at Line 99.
• agents/hermes/mcp-config-transaction.py#L1080-L1083: replace the bare return 8642 default in _service_manager_gateway_public_port with the named default constant, and use the same constant for the module-level GATEWAY_PUBLIC_PORT initializer at Line 182.
• scripts/managed-gateway-control.py#L1136-L1146: replace the bare return 8642 and the port < 8642 or port > 8652 bound in _hermes_api_port with equivalent named constants defined in that script.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@agents/hermes/mcp-config-transaction.py` around lines 102 - 112, Define
shared-named Hermes API port constants next to GATEWAY_INTERNAL_PORT in
agents/hermes/mcp-config-transaction.py, and use them in
_parse_gateway_public_port, _service_manager_gateway_public_port, and the
GATEWAY_PUBLIC_PORT initializer; update agents/hermes/mcp-config-transaction.py
lines 102-112 and 1080-1083 accordingly. Define equivalent constants in
scripts/managed-gateway-control.py and replace the literals in _hermes_api_port
at lines 1136-1146, preserving the existing range validation and default
behavior.

test/hermes-api-port-startup.test.ts (1)

15-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fail fast when a slice anchor is missing.

source.indexOf(...) returns -1 when either anchor text changes in agents/hermes/start.sh. source.slice(start, end) then produces a misleading fragment instead of an explicit error, and the failure surfaces as an unrelated assertion mismatch. The repository already provides extractShellFunction in test/support/hermes-shell-harness.ts, which throws when the pattern does not match. test/hermes-api-port-marker.test.ts uses it.

Either reuse that helper or assert both offsets before slicing.

♻️ Proposed guard

   const start = source.indexOf('NEMOCLAW_CMD=("$@")');
   const end = source.indexOf('\nHERMES="$(command -v hermes)"', start);
+  if (start < 0 || end < 0) {
+    throw new Error("Expected the API port bootstrap block in agents/hermes/start.sh");
+  }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/hermes-api-port-startup.test.ts` around lines 15 - 24, Make the
startup-script extraction fail explicitly when either anchor is missing: update
the setup around NEMOCLAW_CMD and HERMES anchor lookup to reuse
extractShellFunction from the Hermes shell harness, or validate both indexOf
results before calling source.slice. Preserve the existing extracted script
behavior when both anchors are found.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/checks/vitest-project-overlap.mts (1)

46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Use one authoritative installer-test inventory.

The checker and Vitest configuration maintain the same membership manually. This requires synchronized edits and can allow project ownership to drift.

• scripts/checks/vitest-project-overlap.mts#L46-L46: Derive INSTALLER_INTEGRATION_TESTS from the canonical inventory.
• vitest.config.ts#L161-L161: Generate the general integration exclusion from the same inventory.
• vitest.config.ts#L192-L192: Generate the installer-integration inclusion from the same inventory.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/checks/vitest-project-overlap.mts` at line 46, Use one canonical
installer-test inventory and derive all project ownership lists from it. In
scripts/checks/vitest-project-overlap.mts:46-46, define
INSTALLER_INTEGRATION_TESTS from that inventory; in vitest.config.ts:161-161,
generate the general integration exclusion from the same inventory; and in
vitest.config.ts:192-192, generate the installer-integration inclusion from it,
removing duplicated manually maintained membership.

Sources: Coding guidelines, Path instructions

test/hermes-api-port-marker.test.ts (1)

39-50: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Cover the mode half of the runtime-directory guard.

setupUntrustedRuntime reports 1000:1000:755. It exercises only the ownership half of the assertion "must be root-owned with mode 0755". A regression that drops the mode comparison and keeps the owner comparison still passes this file. Add a setup helper that reports 0:0:777 and assert the same non-zero exit and absent marker.

As per path instructions for agents/**: "Require negative-path tests that prove the boundary rejects bypasses."

🛡️ Proposed additional helper and test

+function setupWorldWritableRuntime(
+  _runtimeParent: string,
+  runtimeDir: string,
+  _markerPath: string,
+  _targetPath: string,
+): MarkerSetup {
+  fs.mkdirSync(runtimeDir, { recursive: true });
+  return {
+    shellPrelude: ['stat() { printf "%s\\n" "0:0:777"; }', "chown() { return 0; }"],
+    targetPath: null,
+  };
+}

+  it("refuses a root-owned runtime directory with a permissive mode (`#8543`)", () => {
+    const run = runHermesApiPortMarkerPublication(8645, setupWorldWritableRuntime);
+
+    expect(run.result.status).toBe(1);
+    expect(run.result.stderr).toContain("must be root-owned with mode 0755");
+    expect(run.marker).toBeNull();
+  });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/hermes-api-port-marker.test.ts` around lines 39 - 50, Add a separate
untrusted-runtime setup helper that reports root ownership with mode 0777, then
add a negative-path test using it. Assert the command exits non-zero and the
marker remains absent, covering the mode check independently of the existing
setupUntrustedRuntime ownership case.

Source: Path instructions

agents/hermes/mcp-config-transaction.py (1)

1080-1083: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider rejecting an empty NEMOCLAW_HERMES_API_PORT instead of defaulting.

Line 1082 treats an absent variable and a present-but-empty variable the same way and returns 8642. An absent variable is the legacy case. A present-but-empty variable indicates a broken launch and currently resolves to a port that another sandbox may own. Splitting the two conditions keeps the legacy default and fails closed on the malformed case.

♻️ Proposed change

-    if not values or not values[0]:
+    if not values:
         return 8642
+    if not values[0]:
+        raise PermissionError("Hermes service-manager API port is malformed")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@agents/hermes/mcp-config-transaction.py` around lines 1080 - 1083, Update the
port-resolution logic to distinguish an unset NEMOCLAW_HERMES_API_PORT from a
present-but-empty value: preserve the 8642 legacy default only when the variable
is absent, and reject an explicitly empty value with an appropriate error
instead of selecting a port.

test/hermes-api-port-startup.test.ts (2)

44-62: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

No suite exercises the inclusive edges of the 8642-8652 range. The range is re-implemented in the shell startup path, the shell resolver in scripts/install.sh, and the Python resolver. Each suite tests one interior port and one far-outside port, so an off-by-one in any single implementation stays undetected.

• test/hermes-api-port-startup.test.ts#L44-L62: add 8642 and 8652 as accepted ports, and add the malformed class of non-numeric, empty, and whitespace-padded values to the rejected cases.
• src/lib/actions/uninstall/hermes-forward-watcher-installer.test.ts#L86-L91: add registry cases for hermesApiPort 8642 and 8652 with status 0, and cases for 8641 and 8653 with status 1.

As per path instructions for agents/**: "Require negative-path tests that prove the boundary rejects bypasses."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/hermes-api-port-startup.test.ts` around lines 44 - 62, Expand
test/hermes-api-port-startup.test.ts#L44-L62 to accept boundary ports 8642 and
8652 and reject non-numeric, empty, and whitespace-padded values. In
src/lib/actions/uninstall/hermes-forward-watcher-installer.test.ts#L86-L91, add
registry cases accepting 8642 and 8652 with status 0 and rejecting 8641 and 8653
with status 1, covering both inclusive boundaries and their bypasses.

Source: Path instructions

---

16-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fail loudly when the slice anchors are not found.

indexOf returns -1 when agents/hermes/start.sh no longer contains NEMOCLAW_CMD=("$@") or the HERMES= line. source.slice(-1, end) then yields an almost empty fragment, and the generated script fails with an unrelated bash error such as an unbound PUBLIC_PORT. The sibling file test/hermes-api-port-marker.test.ts uses extractShellFunction from test/support/hermes-shell-harness.ts, which throws a named error instead. Reuse that helper, or assert both indices before slicing.

♻️ Proposed guard

   const start = source.indexOf('NEMOCLAW_CMD=("$@")');
   const end = source.indexOf('\nHERMES="$(command -v hermes)"', start);
+  if (start < 0 || end < 0) {
+    throw new Error("Expected the API port bootstrap block in agents/hermes/start.sh");
+  }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/hermes-api-port-startup.test.ts` around lines 16 - 24, Update the test
setup around the source.slice call to fail immediately when either
NEMOCLAW_CMD=("$@") or the HERMES command anchor is missing. Prefer reusing
extractShellFunction from hermes-shell-harness.ts as in
hermes-api-port-marker.test.ts; otherwise assert both indexOf results are
non-negative before slicing and report the missing anchor clearly.

test/hermes-mcp-api-port.test.ts (1)

33-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Cover _read_service_manager_environment instead of always replacing it.

Every case replaces _read_service_manager_environment. The real function enforces MAX_SERVICE_MANAGER_ENVIRONMENT_BYTES and converts a missing /proc/<pid>/environ into a PermissionError. Neither guard runs in this file. A regression that removes the size cap passes. Add a case that calls the real function against a temporary file path, or add a case that asserts the oversize and missing-file errors.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/hermes-mcp-api-port.test.ts` around lines 33 - 38, Extend the tests
around _read_service_manager_environment to exercise the real implementation
rather than only the monkeypatched replacement. Add coverage for the
MAX_SERVICE_MANAGER_ENVIRONMENT_BYTES limit and the PermissionError raised when
the environment source is missing, using a temporary file path or equivalent
fixture; keep the existing gateway-port cases unchanged.

Source: Path instructions

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/lib/onboard/agent-dashboard-forward.test.ts (1)

41-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Make the regression assertion independent of forwarding order.

The toHaveBeenNthCalledWith assertions couple this test to the current forwarding order and array order. The negative toHaveBeenCalledWith(..., expect.anything()) assertion can miss a default-port call when the call has no third argument. Capture the forwarded URLs, assert that 8643 is present and 8642 is absent, and use expect.objectContaining for the allocated-port options.

Proposed diff

+    const forwardedUrls: string[] = [];
     const ensureDashboardForward = vi.fn((_sandboxName, chatUiUrl = "http://127.0.0.1:18789") => {
+      forwardedUrls.push(chatUiUrl);
       const parsed = new URL(chatUiUrl);
       return Number(parsed.port);
     });
...
-    expect(ensureDashboardForward).toHaveBeenNthCalledWith(1, "hm", "http://127.0.0.1:18789", {
-      preserveSandboxPorts: [18789, 8643, 3978],
-    });
-    expect(ensureDashboardForward).toHaveBeenNthCalledWith(2, "hm", "http://127.0.0.1:8643", {
-      preserveSandboxPorts: [18789, 8643, 3978],
-      allowPortReallocation: false,
-    });
-    expect(ensureDashboardForward).not.toHaveBeenCalledWith(
-      "hm",
-      "http://127.0.0.1:8642",
-      expect.anything(),
+    expect(forwardedUrls).toContain("http://127.0.0.1:8643");
+    expect(forwardedUrls).not.toContain("http://127.0.0.1:8642");
+    expect(ensureDashboardForward).toHaveBeenCalledWith(
+      "hm",
+      "http://127.0.0.1:8643",
+      expect.objectContaining({
+        preserveSandboxPorts: expect.arrayContaining([18789, 8643, 3978]),
+        allowPortReallocation: false,
+      }),
     );

As per path instructions: Review tests for behavioral confidence rather than implementation lock-in, and prefer observable outcomes through the public boundary over mock-call assertions.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/onboard/agent-dashboard-forward.test.ts` around lines 41 - 70, Update
the assertions around ensureAgentDashboardForward and ensureDashboardForward to
capture forwarded URLs from the mock calls, then assert that port 8643 is
present and 8642 is absent without relying on call order. Match allocation
options with expect.objectContaining, and ensure the absence check also covers
calls without a third argument.

Source: Path instructions

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes